fix: adapt code interpreter warm pool to agent-sandbox v0.4.6#387
Conversation
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates Kubernetes/agent-sandbox dependencies and adjusts workload manager + e2e logic to support newer warm-pool controller ownership patterns and SandboxClaim adoption, while also refreshing build/codegen tooling.
Changes:
- Refactor warm-pool pod counting/ready checks to support both direct Pod ownership and SandboxWarmPool → Sandbox → Pod ownership.
- Update sandbox creation flow to handle SandboxClaim adoption by polling claim/sandbox readiness and storing claim identity correctly.
- Bump Kubernetes/tooling versions, regenerate CRDs, and update Docker build images.
Reviewed changes
Copilot reviewed 13 out of 18 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/e2e_test.go | Refactors warm-pool pod discovery to handle new/old ownership chains and reuses shared listing logic. |
| pkg/workloadmanager/sandbox_helper.go | Adds CreatedAt to placeholder sandbox store entries. |
| pkg/workloadmanager/k8s_client.go | Adds dynamic-client getters for Sandbox and SandboxClaim. |
| pkg/workloadmanager/handlers_test.go | Updates annotation constant usage and adds coverage for SandboxClaim-adoption behavior. |
| pkg/workloadmanager/handlers.go | Splits “wait for ready” paths for direct sandboxes vs claim-adopted sandboxes; fixes stored identity for claims. |
| pkg/workloadmanager/codeinterpreter_controller_test.go | Adds tests ensuring SandboxTemplate network policy management is set to unmanaged. |
| pkg/workloadmanager/codeinterpreter_controller.go | Forces SandboxTemplate NetworkPolicyManagement to unmanaged and updates existing templates accordingly. |
| manifests/charts/base/crds/runtime.agentcube.volcano.sh_agentruntimes.yaml | Regenerated CRD schema changes (likely from dependency/codegen bump). |
| hack/update-codegen.sh | Updates code-generator version and changes module discovery approach. |
| go.mod | Updates Go version and bumps k8s/controller-runtime/agent-sandbox deps. |
| go.sum | Updates module sums for the dependency/tooling bumps. |
| docker/Dockerfile.router | Updates Go builder image version. |
| docker/Dockerfile.picod | Updates Go builder image and hardens apt install layer. |
| docker/Dockerfile | Updates Go builder image version. |
Files not reviewed (4)
- client-go/clientset/versioned/fake/clientset_generated.go: Generated file
- client-go/informers/externalversions/factory.go: Generated file
- client-go/informers/externalversions/runtime/v1alpha1/agentruntime.go: Generated file
- client-go/informers/externalversions/runtime/v1alpha1/codeinterpreter.go: Generated file
Comments suppressed due to low confidence (1)
pkg/workloadmanager/sandbox_helper.go:60
- When
CreationTimestampis zero,createdAtis set fromtime.Now(), butexpiresAtuses a separatetime.Now()call. To keep timestamps consistent (and avoid tiny negative/odd deltas in tests/metrics), compute the default expiry fromcreatedAt(e.g.,createdAt.Add(DefaultSandboxTTL)).
createdAt := sandboxCR.GetCreationTimestamp().Time
if createdAt.IsZero() {
createdAt = time.Now()
}
var expiresAt time.Time
if sandboxCR.Spec.Lifecycle.ShutdownTime != nil {
expiresAt = sandboxCR.Spec.Lifecycle.ShutdownTime.Time
} else {
expiresAt = time.Now().Add(DefaultSandboxTTL)
}
| return nil | ||
| } | ||
|
|
||
| func (s *Server) waitForDirectSandboxReady(ctx context.Context, sandbox *sandboxv1alpha1.Sandbox, resultChan <-chan SandboxStatusUpdate) (*sandboxv1alpha1.Sandbox, error) { |
There was a problem hiding this comment.
Thanks, this was a valid point. I added an explicit resultChan == nil guard so a direct sandbox watcher wiring issue now returns errSandboxReadyWatcherNotRegistered immediately instead of being reported as a generic 2-minute sandbox creation timeout.
I also covered the related edge cases in TestWaitForDirectSandboxReadyWatcherFailures:
- nil watcher channel
- closed watcher channel
- watcher event with a nil sandbox
This keeps the direct sandbox path failure mode immediate and diagnosable.
| select { | ||
| case result := <-resultChan: |
| // if warmpool is used, the pod name is stored in sandbox's annotation `agents.x-k8s.io/pod-name` | ||
| sandboxNameForPod := createdSandbox.Name | ||
| sandboxPodName := createdSandbox.Name | ||
| if podName, exists := createdSandbox.Annotations[sandboxv1alpha1.SandboxPodNameAnnotation]; exists { |
| warmPoolSandboxes := make(map[string]struct{}, len(sandboxList.Items)) | ||
| for _, sandbox := range sandboxList.Items { |
| for _, owner := range sandbox.OwnerReferences { | ||
| if owner.Kind == ownerKindSandboxWarmPool && owner.Name == codeInterpreterName { | ||
| warmPoolSandboxes[sandbox.Name] = struct{}{} |
| _, ownedByWarmPoolSandbox := warmPoolSandboxes[owner.Name] | ||
| if (owner.Kind == ownerKindSandboxWarmPool && owner.Name == codeInterpreterName) || | ||
| (owner.Kind == "Sandbox" && ownedByWarmPoolSandbox) { |
| func (f *recordingStore) UpdateSandbox(ctx context.Context, sandbox *types.SandboxInfo) error { | ||
| f.fakeStore.UpdateSandbox(ctx, sandbox) | ||
| if f.updateErr != nil { | ||
| return f.updateErr | ||
| } | ||
| copied := *sandbox | ||
| f.lastUpdated = &copied | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Thanks, this was a valid test-fake issue. I updated recordingStore.UpdateSandbox to propagate the embedded store error and to deep-copy EntryPoints before saving lastUpdated.
This prevents the test from hiding an UpdateSandbox failure or asserting against a later-mutated slice.
5316358 to
5867183
Compare
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #387 +/- ##
==========================================
+ Coverage 58.41% 59.90% +1.48%
==========================================
Files 36 36
Lines 3463 3589 +126
==========================================
+ Hits 2023 2150 +127
+ Misses 1231 1219 -12
- Partials 209 220 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hi @ranxi2001, thanks for your work. First, this PR looks more like a feature than a bug fix. Please confirm and update the PR label if needed.
Could you elaborate on this a bit more? Specifically, at which step would this become a problem? |
|
/remove-kind bug |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 15 changed files in this pull request and generated 2 comments.
Files not reviewed (4)
- client-go/clientset/versioned/fake/clientset_generated.go: Generated file
- client-go/informers/externalversions/factory.go: Generated file
- client-go/informers/externalversions/runtime/v1alpha1/agentruntime.go: Generated file
- client-go/informers/externalversions/runtime/v1alpha1/codeinterpreter.go: Generated file
| echo "Ensuring code-generator@${CODEGEN_VERSION} is available..." | ||
| go get -d "k8s.io/code-generator@${CODEGEN_VERSION}" || true | ||
| CODEGEN_JSON=$(go mod download -json "k8s.io/code-generator@${CODEGEN_VERSION}") | ||
|
|
||
| # Find code-generator in module cache | ||
| CODEGEN_PKG=$(go list -m -f '{{.Dir}}' "k8s.io/code-generator@${CODEGEN_VERSION}" 2>/dev/null || echo "") | ||
| CODEGEN_PKG=$(printf '%s\n' "${CODEGEN_JSON}" | sed -n 's/^[[:space:]]*"Dir": "\([^"]*\)".*/\1/p') |
| // IsWatchListSemanticsSupported informs the reflector that this client | ||
| // doesn't support WatchList semantics. | ||
| // | ||
| // This is a synthetic method whose sole purpose is to satisfy the optional | ||
| // interface check performed by the reflector. | ||
| // Returning true signals that WatchList can NOT be used. | ||
| // No additional logic is implemented here. |
bc8e85b to
c2633c5
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 15 changed files in this pull request and generated 1 comment.
Files not reviewed (4)
- client-go/clientset/versioned/fake/clientset_generated.go: Generated file
- client-go/informers/externalversions/factory.go: Generated file
- client-go/informers/externalversions/runtime/v1alpha1/agentruntime.go: Generated file
- client-go/informers/externalversions/runtime/v1alpha1/codeinterpreter.go: Generated file
| // IsWatchListSemanticsSupported informs the reflector that this client | ||
| // doesn't support WatchList semantics. | ||
| // | ||
| // This is a synthetic method whose sole purpose is to satisfy the optional | ||
| // interface check performed by the reflector. | ||
| // Returning true signals that WatchList can NOT be used. | ||
| // No additional logic is implemented here. |
|
thanks |
|
@ranxi2001 could you please resolve the conflict? |
|
Reviewer note: why the E2E change is larger than a version bump. The previous job installed flowchart LR
A["e2e-test<br/>mTLS enabled"] --> B["AgentRuntime<br/>Router to WorkloadManager mTLS"]
C["codeinterpreter-e2e-test<br/>mTLS disabled"] --> D["CodeInterpreter session"]
D --> E["SandboxClaim"]
E --> F["adopted Sandbox"]
F --> G["pre-warmed Pod<br/>same UID"]
G --> H["session delete"]
H --> I["Claim, Sandbox, and Pod removed<br/>warm pool refilled"]
Both jobs install and verify |
|
/label tide/merge-method-squash |
| } | ||
|
|
||
| podIP, err := s.k8sClient.GetSandboxPodIP(ctx, sandbox.Namespace, sandbox.Name, sandboxPodName) | ||
| podIP, err := s.k8sClient.GetSandboxPodIP(ctx, createdSandbox.Namespace, sandboxNameForPod, sandboxPodName) |
There was a problem hiding this comment.
This reads the claim and Sandbox from the live API, then switches to a separate Pod informer cache that may still lag the Ready status update. A valid warm claim can therefore fail once and be rolled back; please live-GET the annotated Pod or retry Pod discovery until the context expires.
There was a problem hiding this comment.
Thanks, reproduced this with a live API Pod at Running/IP while the informer still held a Pending/no-IP copy. Explicit Pod names now use a live API GET before IP validation, while the label-cache fallback remains for lookups without a Pod name. I also added a regression test for the stale-cache case.
| defer ticker.Stop() | ||
|
|
||
| for { | ||
| claim, err := getSandboxClaim(ctx, dynamicClient, sandboxClaim.Namespace, sandboxClaim.Name) |
There was a problem hiding this comment.
This retries every GET error until the two-minute timer, including permanent Forbidden or conversion errors, and then reports them as a readiness timeout. Please retry only NotFound/transient API errors and return permanent failures immediately.
There was a problem hiding this comment.
Thanks, reproduced this with a Forbidden Claim GET being masked by the readiness timeout. Claim and adopted Sandbox reads now retry only NotFound and explicit transient API/transport errors; Forbidden, Unauthorized, and conversion failures return immediately as sanitized internal errors. Focused tests cover both read stages and the retry classification.
4d5d9fc to
baebe09
Compare
Signed-off-by: ranxi2001 <ranxi169@163.com>
baebe09 to
95fae1f
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: RainbowMango The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR updates AgentCube's CodeInterpreter warm pool integration from
agent-sandbox v0.1.1to the current stable Go module release,v0.4.6. In v0.4.6, aSandboxClaimadopts a pre-warmedSandboxand exposes that serving Sandbox through status, so AgentCube must resolve the claim to the adopted Sandbox instead of assuming the claim or template name identifies the runtime Pod.The change updates the dependency and Kubernetes generator stack, uses the public Sandbox Pod annotation, waits for the adopted Sandbox to become ready, and probes its Pod endpoint. AgentCube still stores the claim name for delete and GC while using runtime data from the adopted Sandbox. CodeInterpreter-created templates explicitly use
networkPolicyManagement: Unmanagedto preserve the existing Router and WorkloadManager traffic path. Warm pool e2e discovery now supports both the old direct ownership shape andSandboxWarmPool -> Sandbox -> Pod.Which issue(s) this PR fixes:
Refs #386
Special notes for your reviewer:
v0.4.6and itsv1alpha1APIs. Thev0.5.x/v1beta1migration is separate because it replacesTemplateRefwith requiredWarmPoolRefandReplicaswithOperatingMode.v0.35.4; there is no intentional AgentRuntime API change.c2633c5.Does this PR introduce a user-facing change?: